DEVOPS-3406 - helm: namespace resouces - #1
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The namespace helper currently renders unquoted, which can produce invalid Kubernetes manifests for numeric namespace values (YAML scalar type mismatch).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the openwork-ee Helm chart to explicitly set metadata.namespace on all namespaced resources using a new top-level namespace value (defaulting to openwork), preventing helm template | kubectl apply flows from accidentally targeting the current kubectl context namespace.
Changes:
- Adds
namespace: openworkto chart values and introduces anopenwork-ee.namespacehelper (with fallback to.Release.Namespacewhen cleared). - Sets
metadata.namespaceacross all namespaced manifests (including test/migration Jobs and optional Ingress/Inference resources). - Adds a chart render assertion script (
tests/namespace.sh) and surfaces the resolved namespace inNOTES.txtand docs.
File summaries
| File | Description |
|---|---|
| packaging/helm/openwork-ee/values.yaml | Introduces documented top-level namespace value (default openwork). |
| packaging/helm/openwork-ee/templates/_helpers.tpl | Adds openwork-ee.namespace helper for consistent namespace selection. |
| packaging/helm/openwork-ee/templates/configmap.yaml | Pins ConfigMap to resolved namespace. |
| packaging/helm/openwork-ee/templates/secret.yaml | Pins Secret to resolved namespace. |
| packaging/helm/openwork-ee/templates/migration-job.yaml | Pins migration Job to resolved namespace (prevents kube-system mishaps). |
| packaging/helm/openwork-ee/templates/tests/env-probe-job.yaml | Pins Helm test Job to resolved namespace. |
| packaging/helm/openwork-ee/templates/den-api.yaml | Pins den-api Service/Deployment to resolved namespace. |
| packaging/helm/openwork-ee/templates/den-web.yaml | Pins den-web Service/Deployment to resolved namespace. |
| packaging/helm/openwork-ee/templates/inference.yaml | Pins inference Service/Deployment to resolved namespace when enabled. |
| packaging/helm/openwork-ee/templates/ingress.yaml | Pins Ingress to resolved namespace when enabled. |
| packaging/helm/openwork-ee/templates/NOTES.txt | Prints resolved namespace and clarifies namespaced targeting. |
| packaging/helm/openwork-ee/tests/namespace.sh | Adds render-time assertions for default/override/fallback namespace behavior. |
| packaging/helm/openwork-ee/README.md | Documents the new namespace behavior and recommended install/apply flows. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: raul-gherman-modaoperandi <68240333+raul-gherman-modaoperandi@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The namespace helper renders unquoted YAML scalars, which can cause invalid manifests if users set a numeric/bool-like namespace via --set, and the new test script should be updated to assert the corrected quoted rendering.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packaging/helm/openwork-ee/tests/namespace.sh:44
- These assertions look for unquoted
namespace: ...lines. If the namespace helper is fixed to quote the value (to prevent numeric/bool YAML parsing), this script should assert quoted output and ideally add a regression check for a numeric--set namespace=123override.
packaging/helm/openwork-ee/templates/_helpers.tpl:20
openwork-ee.namespacecurrently renders unquoted. If a user sets the namespace via--set namespace=123, Helm will type it as a number and the rendered YAML will emitnamespace: 123(non-string), which the Kubernetes API rejects formetadata.namespace. Quoting here also avoids YAML plain-scalar surprises (e.g.on,yes).
{{- define "openwork-ee.namespace" -}}
{{- .Values.namespace | default .Release.Namespace | quote -}}
{{- end -}}
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The namespace helper is consistently applied across all chart resources and is backed by automated render-assertion tests covering default, override, and fallback behavior.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The namespace helper is consistently applied across all chart templates and is backed by a dedicated render-assertion test covering defaults, overrides, fallbacks, and YAML-typing edge cases.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
namespacevalue (defaultopenwork) to theopenwork-eeHelm chart and setsmetadata.namespacefrom it on all 11 namespaced resources: ConfigMap, Secret, Ingress, migration Job, env-probe test Job, and the den-api / den-web / inference Services and Deployments.openwork-ee.namespacehelper intemplates/_helpers.tplthat falls back to.Release.Namespacewhen the value is cleared.README.md(new "Namespace" section), surfaces the rendered namespace intemplates/NOTES.txt, and adds atests/namespace.shchart check.Why
metadata.namespace, sohelm template ... | kubectl apply -f -pipelines fell back to the kubectl context namespace — the migration Job landed inkube-system.Issue
Scope
packaging/helm/openwork-ee/values.yaml: new documentednamespace: openworkvalue.packaging/helm/openwork-ee/templates/_helpers.tpl: newopenwork-ee.namespacehelper.metadata.namespaceadded inconfigmap.yaml,secret.yaml,migration-job.yaml,ingress.yaml,den-api.yaml(Service + Deployment),den-web.yaml(Service + Deployment),inference.yaml(Service + Deployment),tests/env-probe-job.yaml.templates/NOTES.txt: prints the resolved namespace after install.README.md: new "Namespace" section documenting the value,--namespacealignment forhelm install, and the need to pre-create the namespace inhelm template | kubectl applyflows.tests/namespace.sh: new render-assertion chart check.Out of scope
packaging/helm/openwork-ee/tests/*.share the established proof pattern for this chart (no journey spec covers chart rendering).packaging/helm/openwork-ee/examples/.Testing
Ran
for test in packaging/helm/openwork-ee/tests/*.sh; do bash "$test"; done(with helm v3.16.3 on PATH)helm template openwork-ee packaging/helm/openwork-ee(+ variants:--set ingress.enabled=true --set inference.enabled=true,--set namespace=platform,--set namespace=,--namespace rel-ns) to inspect renderednamespace:fieldsResult
namespace chart checks passed(default render puts all 8 default resources / 11 with ingress+inference inopenwork; override wins everywhere; cleared value falls back to the release namespace).CI status
publish-ee-imagesworkflow chart checks on push)Manual verification
helm template openwork-ee ./packaging/helm/openwork-eeand confirm every manifest hasmetadata.namespace: openwork(including the migration Job).helm template openwork-ee ./packaging/helm/openwork-ee --set namespace=platformand confirm all manifests useplatform.helm template openwork-ee ./packaging/helm/openwork-ee --set namespace= --namespace rel-nsand confirm all manifests fall back torel-ns.Evidence
Risk
metadata.namespaceis declarative and matches the documented--namespace openworkinstall flow. Values-compatibility risk is minimal — existing releases installed with--namespace openwork(per README) render identical effective namespaces; only pipelines that relied on an implicit context namespace different fromopenworkmust now setnamespaceexplicitly.Rollback